Allow passing several reserved state chunks in single process call #124574
Conversation
elasticsearchmachine
added
the
serverless-linked
jfreden
changed the title
jfreden
force-pushed
the
change_mp_file_processing
branch
from
2d34177 to
4095a58
Compare
jfreden
changed the title
| } | ||
|
|
||
| ReservedStateChunk parse(ProjectId projectId, String namespace, XContentParser parser) { | ||
| public ReservedStateChunk parse(ProjectId projectId, String namespace, XContentParser parser) { |
There was a problem hiding this comment.
This can be used by a caller to parse several state chunks before passing them to process.
| return; | ||
| } | ||
|
|
||
| if (errorState.projectId().isPresent() && clusterService.state().metadata().hasProject(errorState.projectId().get()) == false) { |
There was a problem hiding this comment.
This was a bug. If parsing fails for a project that doesn't exist yet, the error state reporting won't work since it's persisted in the project metadata that doesn't exist yet. This case needs to be handled in the caller (multi-project file service).
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @jfreden, I've created a changelog YAML for you. |
| ); | ||
| } | ||
|
|
||
| private static ReservedStateChunk mergeReservedStateChunks(List<ReservedStateChunk> chunks) { |
There was a problem hiding this comment.
Since we are merging the list of chunks into a single ReservedStateChunk, why do we also need to change the process method signature? Maybe I am missing something?
There was a problem hiding this comment.
If we do want to change the process method signature, I'd think it is probably more useful to accept a Map<String, ReservedStateChunk> so that each chunk can be stored in its own namespace rather than a single one. This seems useful to separate regular settings and secrets. It does lead to more code changes. So maybe that is something you are trying to avoid?
There was a problem hiding this comment.
Thanks for pointing this out! I've spent some time thinking about this.
Since we are merging the list of chunks into a single ReservedStateChunk, why do we also need to change the process method signature? Maybe I am missing something?
Before this change process was called with an XContentParser instance (the parser for the json files), once for each file. This is still supported.
With this change we call process once per project to trigger a single cluster state update instead of two and that's why we need to either call process with one parser for each file (I find this approach a little awkward) or call the parse method from the caller MultiProjectFileSettingsService and then pass the resulting ReservedStateChunk instances to process (this is the approach I went with).
If we do want to change the process method signature, I'd think it is probably more useful to accept a Map<String, ReservedStateChunk> so that each chunk can be stored in its own namespace rather than a single one. This seems useful to separate regular settings and secrets. It does lead to more code changes. So maybe that is something you are trying to avoid?
The purpose of this change is to be able to spread the state for a single namespace across several chunks. Since reserved state is versioned per namespace I think it makes the most sense to have a single one for both.
The alternative I think you're proposing is to have several reserved state namespaces in a single cluster state update. We'd then have to add version dependencies between namespaces, which I think is out of scope for the ReservedClusterStateService but could definitely be implemented in the MultiProjectFileSettingsService. I'm leaning towards sticking with a single namespace, but happy to discuss this further.
There was a problem hiding this comment.
Before this change process was called with an XContentParser instance
Thanks for explaining. I see the issue now.
The alternative I think you're proposing is to have several reserved state namespaces in a single cluster state update.
After commenting on the other PR about passing down both project settings and secrets at the same time, I now think it might be better to use a single namespace as you have proposed. Sorry for the back and forth.
There was a problem hiding this comment.
I think it makes sense. I'm struggling a little with the biger picture. Does the control plane logic need to exercise special care to make sure all the files arrive "at the same time"? What happens if there's some lag between when some files arrive versus others, and the processing happens inbetween?
| try { | ||
| reservedStateChunk = reservedStateChunks.size() == 1 | ||
| ? reservedStateChunks.getFirst() | ||
| : mergeReservedStateChunks(reservedStateChunks); |
There was a problem hiding this comment.
Minor: Seems to me mergeReservedStateChunks could handle this case for you. Then the caller doesn't need a ternary.
There was a problem hiding this comment.
Yes, that's nicer! Thanks!
If one file comes in before the other, the processing will fail and be retried when all files are there. We will try to apply reserved state for every file change. |
jfreden
added
the
auto-merge-without-approval
…lastic#124574) This PR overloads the `process` method and allows it to be used with several `ReservedStateChunks`. The purpose is to allow several state chunks to be spread across several files but handled as a single cluster state update by validating and merging them into a single representation of the `ReservedStateChunk`.
…lastic#124574) This PR overloads the `process` method and allows it to be used with several `ReservedStateChunks`. The purpose is to allow several state chunks to be spread across several files but handled as a single cluster state update by validating and merging them into a single representation of the `ReservedStateChunk`.
…lastic#124574) This PR overloads the `process` method and allows it to be used with several `ReservedStateChunks`. The purpose is to allow several state chunks to be spread across several files but handled as a single cluster state update by validating and merging them into a single representation of the `ReservedStateChunk`.
4 participants
This PR overloads the
processmethod and allows it to be used with severalReservedStateChunks. The purpose is to allow several state chunks to be spread across several files but handled as a single cluster state update by validating and merging them into a single representation of theReservedStateChunk.